[톰캣 구현하기 3, 4단계] 루카(백경환) 미션 제출합니다. #489

dooboocookie · 2023-09-11T08:15:36Z

안녕하세요 25..............

아직 테스트 보완이 필요한 상황이긴 한데요. 리뷰 적용과 같이 테스트도 보완하겠습니다.

요구사항 정리와 그를 반영하는 테스트
쓰레드 관련 테스트

그리고 전체적인 구성도는 다음과 같습니다!!

다음 리뷰에는 요청 흐름에 대해서도 올려보겠습니다!!
잘부탁드립니다!!

LJW25

안녕하세요 루카, 이오입니다! 🐕‍🦺🐩
3, 4단계 구현하느라 수고 많으셨습니다. 👏

구성도도 그려주셔서 이해하는데 도움이 되었어요👍
몇가지 궁금한 것도 있고, 오류로 보이는 부분도 있어서 리뷰 남겼으니 확인부탁드립니다. 😁
오류는 아니지만 컨벤션(공백, 접근제어자 등)이 지켜지지 않은 부분은 #컨벤션으로 남겨놓았는데 참고만 해주시고 반영은 안하셔도 됩니다ㅎㅎ

이해가 가지 않는 리뷰가 있다면 DM 주세요.
리팩토링 화이팅❗️❗️

LJW25 · 2023-09-11T10:30:19Z

tomcat/src/main/java/org/apache/coyote/controller/AbstractController.java

+            doPost(request, response);
+            return;
+        }
+        response.setStatusCode(METHOD_NOT_ALLOWED);


405 에러까지 처리하셨네요 👍👍

LJW25 · 2023-09-11T10:30:55Z

tomcat/src/main/java/nextstep/jwp/controller/HomeController.java

+    @Override
+    protected void doPost(final Request request, final Response response) {
+        response.responseNotFound();
+    }


405 에러를 만드셨는데, 해당 상황을 404로 처리하신 이유가 있을까요?

헉 해당 부분은 실수 입니다. 수정하겠습니다.

LJW25 · 2023-09-11T10:35:27Z

tomcat/src/main/java/org/apache/coyote/http11/response/Response.java

+    public void redirect(final String path) {
+        this.statusLine.setStatusCode(FOUND);
+        final Headers redirectHeaders = new Headers();
+        redirectHeaders.addHeader(LOCATION, path);
+        this.headers = redirectHeaders;
+    }


redirect 메소드에서 setStatusCode(FOUND)를 수행하는 이유가 있으실까요?

질문의 의도를 제대로 파악하지 못했습니다!! ㅠㅠㅠㅠ
그래도 제가 최대한 이해한대로 말씀드리면,
이 리다이렉트 메서드는 3xx대의 응답 코드와 Location헤더를 같이 내려주서 path로 준 요청으로 리다이렉트 하는 응답으로 세팅하는 역할을 합니다!!
현재는 요구사항에선 FOUND 응답 상태로 내려가라고 되어있어서 이렇게 진행했습니아!!

앗 제가 너무 생략하고 말씀드렸군요!
저는 redirect 라는 메소드가 redirect 헤더를 추가해주는 것뿐 아닌, 상태코드를 수정하는 것이 해당 메소드의 책임이 맞을까 하는 방향에서 드린 질문이었습니다.
혼동을 드려 죄송합니다. 🥲

LJW25 · 2023-09-11T10:42:49Z

tomcat/src/main/java/org/apache/catalina/connector/Connector.java

+    }
+
+    public Connector(final int port, final int acceptCount, final int maxThreads) {
+        int coreThreadCount = Runtime.getRuntime().availableProcessors() * 2;


#컨벤션 이 변수도 final 붙일 수 있을것같아요!

수정했습니다!!

LJW25 · 2023-09-11T10:44:25Z

tomcat/src/main/java/nextstep/jwp/controller/RegisterController.java

+        final String account = request.getParameter("account")
+                .orElseThrow(() -> new IllegalArgumentException("계정 입력이 잘못되었습니다."));
+        final String password = request.getParameter("password")
+                .orElseThrow(() -> new IllegalArgumentException("비밀번호 입력이 잘못되었습니다."));
+        final String email = request.getParameter("email")
+                .orElseThrow(() -> new IllegalArgumentException("이메일 입력이 잘못되었습니다."));


꼼꼼한 예외처리 👍
LoginController에서는 다르게 처리하신 이유가 있으실까요?

final Optional<User> maybeUser = InMemoryUserRepository.findByAccount(account.get()); if (maybeUser.isEmpty()) { response.setStatusCode(UNAUTHORIZED); response.writeStaticResource("/401.html"); return; } final User findUser = maybeUser.get(); final Optional<String> password = request.getParameter("password"); if (password.isEmpty() || !findUser.checkPassword(password.get())) { response.setStatusCode(UNAUTHORIZED); response.writeStaticResource("/401.html"); return; }

LoginController는 해당 파라미터가 있냐 없냐에 따라서 이 인증 처리의 어떻게 분기 처리할지 갈리게 되므로 그 자체를 판단하기 위해서 Optional 값이 존재하는지 확인합니다.

하지만 위의 회원가입에서는 그 입력값이 들어오지 않았던 것이므로 예외처리를 하게됩니다.

만약 스프링으로 옮긴다면
LoginController에서 리퀘스트 바디를 바인딩하다 생긴 에러는 익셉션 핸들러에 의해서 401응답을 내려주는 과정을 거쳤을 것 같습니다

LJW25 · 2023-09-11T11:03:48Z

tomcat/src/test/java/nextstep/org/apache/coyote/http11/Http11ProcessorTest.java

+        assertAll(() -> {
+            assertThat(actual).contains("HTTP/1.1 200 OK");
+            assertThat(actual).contains("Content-Type: text/html;charset=utf-8");
+            //assertThat(actual).contains("Content-Length: 5564");


#컨벤션 여기도 안쓰이는 주석이 있어요!

제거하겠습니다!

LJW25 · 2023-09-11T11:05:38Z

tomcat/src/test/java/org/apache/coyote/http11/request/RequestUriTest.java

+        //given
+        final String requestUriValue = "/path?a=1&b=2";
+        //when
+        RequestUri requestUri = RequestUri.from(requestUriValue);


#컨벤션 given과 when 사이에 공백이 빠졌어요!
그리고 when에서도 final 을 붙일 수 있을 것 같습니다.

다른 테스트에서도 when에 final 이 없는 경우가 많던데, final 을 사용할거라면 통일해서 모두 붙여주는건 어떨까요? (꼭 반영하실 필요는 없어요!)

꼼꼼한 리뷰 정말 감사합니다..
이런 부분을 신경쓰면서했어야는데 ㅠㅠ

LJW25 · 2023-09-11T11:06:26Z

tomcat/src/test/java/org/apache/coyote/http11/request/RequestUriTest.java

+        //then
+        assertSoftly(softAssertions -> {
+            softAssertions.assertThat(requestUri.getPath()).isEqualTo("/path");
+            softAssertions.assertThat(requestUri.getQueryString()).isEqualTo("a=1&b=2");
+        });


다른 테스트에서는 assertAll을 사용하셨는데 여기서만 assertSoftly를 사용하신 이유가 있을까요?

원래는 assertSoftly를 선호하는 편입니다!
이 메서드가 어떤 지점에서 에러가 낫는지 추적하기 훨씬 좋더라구요!! 제가 급하게 테스트를 짜다보니 다르게 적용한 것 같은데 전부 일치시키겠습니다!!

LJW25 · 2023-09-11T11:12:36Z

tomcat/src/main/java/org/apache/coyote/http11/response/Response.java

+    public void responseUnauthorized() {
+        this.statusLine.setStatusCode(UNAUTHORIZED);
+        this.headers = new Headers();
+        this.body = "";
+    }
+
+    public void responseNotFound() {
+        this.statusLine.setStatusCode(NOT_FOUND);
+        this.headers = new Headers();
+        this.body = "";
    }


401, 404 오류가 발생한 경우에 응답 html 페이지는 왜 내려주지 않나요? (진짜 궁금해서 드리는 질문🤔)

어... 실수 입니다...
제가 해당 에러에 응답해줘야될 에러페이지가 있다는 사실을 망각했습니다.
Rest API로 착각해서 그에 대한 응답을 내려줬던 것입니다.
그리고 특정 응답을 세팅하는 메서드가 굳이 필요한 것 같지 않아보이므로 제거하도록하겠습니다.

LJW25 · 2023-09-11T11:14:44Z

tomcat/src/main/java/org/apache/coyote/http11/response/StatusCode.java

@@ -3,9 +3,11 @@
 public enum StatusCode {

    OK(200, "OK"),
+    CREATED(201, "CREATED"),
    FOUND(302, "FOUND"),
    UNAUTHORIZED(401, "UNAUTHORIZED"),
    NOT_FOUND(404, "NOT FOUND"),


남길곳이 애매해서 여기에 남깁니다!

현재 잘못된 path를 입력할 경우 404 오류가 발생하지 않고, 그냥 알수없는 예외로 처리되는 것 같아요!
미션 요구사항은 아니었기 때문에 상관은 없지만, 혹시 의도하신 바일까요?

public static Controller getController(final Request request) { final String path = request.getRequestLine().getRequestPath(); if (CONTROLLER_MAP.containsKey(path)) { return CONTROLLER_MAP.get(path); } return STATIC_RESOURCE_CONTROLLER; }

컨트롤러 매퍼에서 컨트롤러를 조회할 때 절차(우선순위)를 줘서 처리해보았습니다.

먼저 패스가 매핑되었는지 확인한다.

매핑이 안된 패스는 정적 파일에서 찾는다.

정적 파일에 존재하지 않으면 404응답하도록 한다.

… 않도록 수정

sonarcloud · 2023-09-12T17:24:48Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

0.0% Coverage
0.0% Duplication

The version of Java (11.0.20.1) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more here

dooboocookie · 2023-09-12T17:38:00Z

안녕하세요 이오!!
정말 꼼꼼한 리뷰 감사합니다!
덕분에 세세한 부분까지 잘 인지하면서 수정할 수 있었습니다!!

LJW25

안녕하세요 루카!

자잘한 수정 요청이 많았는데 잘 반영해주셔서 감사합니다 :)
redirect 관련해서 추가 코멘트 남겼어요. 처음 질문에서 너무 생략하고 말씀드려 죄송해요 😂
리팩토링 너무너무 수고하셨습니다.
루카 덕분에 리뷰하면서 많이 배운것 같아요.

다음 미션도 화이팅 하세요!! 😆

dooboocookie added 16 commits September 11, 2023 17:08

feat: RequestUri 구현

53bc8ba

test: RequestLine 테스트 추가

6af43a0

test: Request 테스트 추가

16b4559

test: RequestParameters 테스트 추가

d8321b2

test: Headers 테스트 추가

b0f0ae3

test: StatusLine 테스트 추가

dfd1221

test: Response 테스트 보완 및 리팩토링

3e93477

feat: Response가 Cookie 추가하는 기능 갖도록 수정

b364094

chore: 코드 스타일 정리 및 임포트문 정리

b1db534

refactor: 리퀘스트 파라미터 Optional로 반환하도록 변경

be4949d

refactor: Controller 구현

5328934

feat: ThreadPool 설정 추가

11656cd

feat: ConcurrentHashMap 변경

5224c13

chore: 코드 컨벤션 추가

0a74fe5

chore: 리다이렉트 리턴 안하던 과정 추가

1f109f6

chore: 코드 스멜 제거

8153bdc

LJW25 requested changes Sep 11, 2023

View reviewed changes

dooboocookie added 6 commits September 12, 2023 22:42

fix: POST / 요청 405 응답으로 수정

b05c414

refactor: Response에서 401, 404 응답 세팅하는 메서드 삭제

73616d0

refactor: 없는 정보는 404응답 주도록 수정

549b696

fix: 이미 요청이 SessionId를 포함하고 있고 그 SessionId를 갱신하지 않았다면 SessionId를 갱신하지…

941a808

… 않도록 수정

test: SoftAssertions로 수정

de558a1

chore: 컨벤션 수정

b16b372

This comment was marked as duplicate.

Sign in to view

LJW25 approved these changes Sep 13, 2023

View reviewed changes

LJW25 merged commit 5274c22 into woowacourse:dooboocookie Sep 13, 2023
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[톰캣 구현하기 3, 4단계] 루카(백경환) 미션 제출합니다. #489

[톰캣 구현하기 3, 4단계] 루카(백경환) 미션 제출합니다. #489

dooboocookie commented Sep 11, 2023

LJW25 left a comment

LJW25 Sep 11, 2023

LJW25 Sep 11, 2023

dooboocookie Sep 12, 2023

LJW25 Sep 11, 2023

dooboocookie Sep 12, 2023

LJW25 Sep 13, 2023

LJW25 Sep 11, 2023

dooboocookie Sep 12, 2023

LJW25 Sep 11, 2023

dooboocookie Sep 12, 2023

LJW25 Sep 11, 2023

dooboocookie Sep 12, 2023

LJW25 Sep 11, 2023

dooboocookie Sep 12, 2023

LJW25 Sep 11, 2023

dooboocookie Sep 12, 2023

LJW25 Sep 11, 2023

dooboocookie Sep 12, 2023

LJW25 Sep 11, 2023

dooboocookie Sep 12, 2023 •

edited

Loading

sonarcloud bot commented Sep 12, 2023

dooboocookie commented Sep 12, 2023

This comment was marked as duplicate.

LJW25 left a comment

[톰캣 구현하기 3, 4단계] 루카(백경환) 미션 제출합니다. #489

[톰캣 구현하기 3, 4단계] 루카(백경환) 미션 제출합니다. #489

Conversation

dooboocookie commented Sep 11, 2023

LJW25 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dooboocookie Sep 12, 2023 • edited Loading

Choose a reason for hiding this comment

sonarcloud bot commented Sep 12, 2023

dooboocookie commented Sep 12, 2023

This comment was marked as duplicate.

LJW25 left a comment

Choose a reason for hiding this comment

dooboocookie Sep 12, 2023 •

edited

Loading